Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Middleware server group power ops #13741

Merged
merged 4 commits into from
Feb 14, 2017

Conversation

jkremser
Copy link
Member

@jkremser jkremser commented Feb 2, 2017

Adding support for server group operations, namely:

  • suspend
  • resume
  • restart
  • stop
  • start

@miq-bot add_labels providers/hawkular, enhancement

Copy link

@bronaghs bronaghs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jiri-Kremser
Just a couple of very minor questions, otherwise looks great.

def shutdown_middleware_server(ems_ref, params)
# server group ops
def start_middleware_server_group(ems_ref)
# blocking: boolean

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jiri-Kremser - what does this comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the optional parameters the operation can take, I can remove that if it's confusing. Or comment better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted an optional param wouldn't you need
def start_middleware_server_group(ems_ref, blocking = false) or something like that?

Copy link
Member Author

@jkremser jkremser Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking is an optional param in the underlying Wildfly dmr api, but we never set it to false, so I removed the comment at all and didn't complicate the method signature with it.


def stop_middleware_server_group(ems_ref, params)
# blocking: boolean
# timeout: int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jiri-Kremser - I'm not sure about the timeout comment either...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is params a hash that is expected to have {:blocking, :timeout} as possible keys?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. Possible keys that we are not using now. I am ok with removing the comments or commenting it better.

Copy link
Member

@agrare agrare Feb 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh duh I didn't see this timeout = params[:timeout] || 0 below
I think I'd leave the comments out and if timeout is optional, make params = {} so it is optional as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the comments and made the params hash optional arg. for couple of methods.

# server ops
def shutdown_middleware_server(ems_ref, params)
timeout = params[:timeout] || 0
run_generic_operation(:Shutdown, ems_ref, :restart => false, :timeout => timeout)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jiri-Kremser - is there a default value for :restart? I see it is false here and true below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jboss cli says:

./jboss-cli.sh --connect ':read-operation-description(name=shutdown)'
{
    "outcome" => "success",
    "result" => {
        "operation-name" => "shutdown",
        "description" => "Shuts down the server via a call to System.exit(0)",
        "request-properties" => {
            "restart" => {
                "type" => BOOLEAN,
                "description" => "If true, once shutdown the server will be restarted again",
                "expressions-allowed" => false,
                "required" => false,
                "nillable" => true,
                "default" => false
            },
            "timeout" => {
                "type" => INT,
                "description" => "The shutdown timeout in seconds. If this is zero (the default) then the server will shutdown immediately. A value larger than zero means the server will wait up to this many seconds for all active requests to finish. A value smaller than zero means that the server will wait indefinitely for all active requests to finish.",
                "expressions-allowed" => false,
                "required" => false,
                "nillable" => true,
                "default" => 0
            }
        },
        "reply-properties" => {},
        "read-only" => false,
        "runtime-only" => true
    }
}

So the default is false, I guess the author of the code wanted to be explicit here. It's not my code btw. I only moved the methods that belongs together closer to each other. Sorry for that, the readability of the diff might be bad.

@bronaghs
Copy link

@miq-bot assign @agrare

@miq-bot miq-bot assigned agrare and unassigned blomquisg Feb 13, 2017
@jkremser jkremser force-pushed the server-group-power-ops branch from 00c6d6e to 1233aa2 Compare February 14, 2017 13:45
@miq-bot
Copy link
Member

miq-bot commented Feb 14, 2017

Checked commits jkremser/manageiq@fb4df7a~...bfeba58 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

@bronaghs
Copy link

thanks @Jiri-Kremser - looks good.

@agrare
Copy link
Member

agrare commented Feb 14, 2017

LGTM 👍 thanks @Jiri-Kremser !

@agrare agrare merged commit 345a535 into ManageIQ:master Feb 14, 2017
@jkremser jkremser deleted the server-group-power-ops branch February 14, 2017 15:18
@chessbyte chessbyte changed the title Middelware server group power ops Middleware server group power ops Feb 18, 2017
@chessbyte chessbyte added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants